Added: ability to set basic authentication on requests #778
Conversation
There was a problem hiding this comment.
Thanks for this @agamdua! I've left a few notes in the diff. I'm broadly happy with this, but I'd like to see if @FranzBusch thinks the API is suitable for now.
| // | ||
| // This source file is part of the AsyncHTTPClient open source project | ||
| // | ||
| // Copyright (c) 2022 Apple Inc. and the AsyncHTTPClient project authors |
There was a problem hiding this comment.
| // Copyright (c) 2022 Apple Inc. and the AsyncHTTPClient project authors | |
| // Copyright (c) 2024 Apple Inc. and the AsyncHTTPClient project authors |
| /// - username: the username to authenticate with | ||
| /// - password: authentication password associated with the username | ||
| mutating public func setBasicAuth(username: String, password: String) { | ||
| let value = Data(String(format: "%@:%@", username, password).utf8) |
There was a problem hiding this comment.
I think we can construct this a bit more cheaply. Something like this:
var value = Data()
value.reserveCapacity(username.utf8.count + password.utf8.count + 1)
value.append(contentsOf: username.utf8)
value.append(UInt8(ascii: ":"))
value.append(contentsOf: password.utf8)| mutating public func setBasicAuth(username: String, password: String) { | ||
| let value = Data(String(format: "%@:%@", username, password).utf8) | ||
| let encoded = value.base64EncodedString() | ||
| self.headers.replaceOrAdd(name: "Authorization", value: "Basic \(encoded)") |
There was a problem hiding this comment.
Let's refactor out the common code here.
There was a problem hiding this comment.
I created a new file and function for the code here, and extended HTTPHeaders with internal access to reuse the code effectively. Let me know if you prefer different code organization!
|
@swift-server-bot add to allowlist |
There was a problem hiding this comment.
Generally speaking this looks good to me. Just the comments that @Lukasa added.
|
Sounds good - thank you both! Will address comments soon, they all make sense 👍 |
agamdua
force-pushed
the
agam-basic-auth
branch
2 times, most recently
from
a3387b1 to
44fb720
Compare
Motivation: As an HTTP library, async-http-client should have authentication support. Modifications: This adds a `setBasicAuth()` method to both `HTTPClientRequest` and `HTTPClient.Request` and their related unit tests. Result: Library users will be able to leverage this method to use basic authentication on their requests without implementing this in their own projects. Signed-off-by: Agam Dua <[email protected]>
agamdua
force-pushed
the
agam-basic-auth
branch
from
44fb720 to
19ea3d9
Compare
Motivation:
As an HTTP library, async-http-client should have authentication support.
Modifications:
This adds a
setBasicAuth()method to both HTTPClientRequest andHTTPClient.Requestand their related unit tests.Result:
Library users will be able to leverage this method to use basic authentication on their requests without implementing this in their own projects.
Note: I also ran the tests (
swift test) with thedocker.io/library/swift:6.0-focalanddocker.io/library/swift:5.10.1-focalto ensure linux compatibility.